Skip to content

fix: Phase 1 - Critical Security Fixes#2

Closed
AhmedV20 wants to merge 1 commit intomainfrom
fix/phase1-critical-security-fixes
Closed

fix: Phase 1 - Critical Security Fixes#2
AhmedV20 wants to merge 1 commit intomainfrom
fix/phase1-critical-security-fixes

Conversation

@AhmedV20
Copy link
Owner

Phase 1: Critical Security Fixes 🔒

Overview

This PR addresses critical security vulnerabilities identified in the Learnify PRD Phase 1. All changes are focused on enhancing security and production-readiness.


📋 Changes Summary

Category File Change Priority
Security CoursesController.cs Added ownership verification for course updates P0
Security Program.cs Restrictive CORS policy + HTTPS enforcement P0
Config appsettings.json Added AllowedOrigins configuration P0
Config appsettings.Development.json New file for dev-specific settings P0
Build Learnify.Application.csproj Removed duplicate FluentValidation reference P1

🔒 Security Improvements

1. Authorization Fix in CoursesController.Update()

Before:

// TODO: Add authorization check to ensure user owns this course
// For now, we'll let the command handler deal with authorization

After:

// Authorization check: Ensure user is admin or course owner
var isAdmin = User.IsInRole("Admin");
var course = await _mediator.Send(new GetCourseByIdQuery(id));

if (course == null)
    return NotFound();

if (!isAdmin && course.InstructorId != currentUserId)
    return Forbid("Only course owners or administrators can update courses");

Impact: Users can no longer modify courses they don't own.


2. Restrictive CORS Policy

Added:

  • ProductionCors - Restricts to allowed origins from configuration
  • DevelopmentCors - Allows all origins for local testing
  • Environment-based policy switching

Configuration (appsettings.json):

"AllowedOrigins": [
  "http://localhost:3000",
  "http://localhost:5173",
  "https://localhost:3000",
  "https://yourdomain.com"
]

Impact: Eliminates AllowAnyOrigin() security vulnerability.


3. HTTPS Enforcement

Added:

if (!app.Environment.IsDevelopment())
{
    app.UseHttpsRedirection();
    app.UseHsts();
}

Impact: All production traffic is forced to HTTPS.


4. Clean Build

Removed: Duplicate FluentValidation package reference.

Impact: Eliminates potential build conflicts.


🧪 Testing Checklist

  • Test course update with non-owner user → Should return 403 Forbidden
  • Test course update with admin user → Should succeed
  • Test course update with course owner → Should succeed
  • Test CORS in production → Only origins from AllowedOrigins accepted
  • Test CORS in development → All origins allowed
  • Test HTTP request in production → Should redirect to HTTPS
  • Build completes without warnings
  • All existing tests pass

📊 Risk Assessment

Change Risk Mitigation
Authorization check Low Matches existing Delete() pattern
CORS restriction Low Development environment allows all origins
HTTPS enforcement Low Only applies to production
Package cleanup None Removed duplicate only

🔄 Backward Compatibility

Breaking Changes: None

Configuration Required: Yes - Add AllowedOrigins to appsettings.json for production.


📝 Related Issues

  • Closes Phase 1 of Learnify PRD
  • Addresses critical security vulnerabilities
  • Part of prioritized roadmap (Week 1-2)

✨ Next Steps (After Merge)

  • Phase 2: Testing & Quality (Test framework, unit tests, integration tests)
  • Phase 3: Security & Monitoring (JWT blacklisting, CAPTCHA, Sentry)

Reviewer: Please verify:

  1. Authorization logic matches security requirements
  2. CORS configuration is appropriate for production
  3. HTTPS enforcement works as expected
  4. No breaking changes introduced

## Security Fixes

- Fix authorization check in CoursesController.Update() to prevent unauthorized course modifications
- Replace AllowAllOrigins CORS policy with restrictive environment-based configuration
- Add HTTPS enforcement middleware for production environments
- Remove duplicate FluentValidation package reference

## Changes

**src/Learnify.Api/Controllers/CoursesController.cs**
- Add ownership verification before allowing course updates
- Only course owners or admins can modify courses
- Matches authorization pattern used in Delete() method

**src/Learnify.Api/Program.cs**
- Create DevelopmentCors policy (allows all for local testing)
- Create ProductionCors policy (restricted to allowed origins)
- Add HTTPS redirection and HSTS in production
- Switch CORS policy based on environment

**src/Learnify.Api/appsettings.json**
- Add AllowedOrigins configuration section
- Default origins: localhost:3000, localhost:5173, https://yourdomain.com

**src/Learnify.Api/appsettings.Development.json** (new)
- Development-specific configuration
- Allows localhost origins for local development

**src/Learnify.Application/Learnify.Application.csproj**
- Remove duplicate FluentValidation package reference

## Impact

- 🔒 Enhanced security: Users cannot modify courses they don't own
- 🔒 Production-ready CORS: No more AllowAllOrigins vulnerability
- 🔒 HTTPS enforcement: All production traffic forced to HTTPS
- ✨ Clean build: No duplicate package conflicts

## Testing

- Test course update with non-owner user → Should return 403 Forbidden
- Test CORS in production → Only allowed origins accepted
- Test HTTP in production → Should redirect to HTTPS

Closes #Phase1
@AhmedV20 AhmedV20 assigned AhmedV20 and unassigned AhmedV20 Jan 31, 2026
@AhmedV20 AhmedV20 closed this Jan 31, 2026
@AhmedV20 AhmedV20 deleted the fix/phase1-critical-security-fixes branch January 31, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant